Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass mouse button state into HandleMouse instead of asking win32 #6765

Merged
merged 10 commits into from
Aug 7, 2020
Merged

Pass mouse button state into HandleMouse instead of asking win32 #6765

merged 10 commits into from
Aug 7, 2020

Conversation

carlos-zamora
Copy link
Member

Summary of the Pull Request

TerminalInput::HandleMouse now needs an extra parameter to track the state of the mouse buttons.

  • ConHost: parameter defined in windowio
  • Terminal: parameter defined in TermControl --> Terminal

References

#4848 - HandleMouse being moved to TermInput

PR Checklist

Validation Steps Performed

ran mc in Terminal

@ghost ghost added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Input Related to input processing (key presses, mouse, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conpty For console issues specifically related to conpty labels Jul 2, 2020
zadjii-msft
zadjii-msft requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel like these comments really warrant a "block", but I also want github to actually track the last time I reviewed these files.

src/terminal/input/terminalInput.hpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved
src/interactivity/win32/windowio.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 7, 2020
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 9, 2020
@ghost
Copy link

ghost commented Jul 9, 2020

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Jul 9, 2020
@ghost ghost requested review from miniksa, DHowett and leonMSFT July 9, 2020 18:06
DHowett
DHowett reviewed Jul 9, 2020
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lots of parameter copying going on, but i don't totally mind.

src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved
@DHowett
Copy link
Member

DHowett commented Jul 9, 2020

I still want to understand..

what if you are scrolling with buttons pressed? then what? :P

DHowett
DHowett requested changes Jul 9, 2020
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking after discussion with Carlos about passing empty states in when we shouldn't

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 9, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 9, 2020
DHowett
DHowett requested changes Jul 15, 2020
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this have to be three booleans? can it be a button mask so we don't back ourselves into a corner for the future point where somebody asks for buttons 4/5/6/7/8 to work properly? i don't love s_getPressedButton, because it no longer gets a pressed button, it more like "determines the most important button out of the provided button set I think"??

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 15, 2020
@DHowett DHowett removed their assignment Jul 15, 2020
@carlos-zamora
Copy link
Member Author

does this have to be three booleans? can it be a button mask so we don't back ourselves into a corner for the future point where somebody asks for buttons 4/5/6/7/8 to work properly?

So, the main problem here is IMouseWheelListener.idl. I could introduce a struct there for MouseButtonState that is the same as TerminalInput::MouseButtonState. That still seems wrong to me though so idk. :/

i don't love s_getPressedButton, because it no longer gets a pressed button, it more like "determines the most important button out of the provided button set I think"??

This is working the same as before though. All I did was make it take a parameter for MouseButtonState and read from that. :/

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 17, 2020
DHowett
DHowett requested changes Jul 17, 2020
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 17, 2020
@DHowett DHowett removed their assignment Jul 17, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 17, 2020
DHowett
DHowett requested changes Jul 20, 2020
View reviewed changes
src/cascadia/PublicTerminalCore/HwndTerminal.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/terminal/input/terminalInput.hpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 20, 2020
@DHowett DHowett removed their assignment Jul 20, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 21, 2020
@ghost ghost requested a review from PankajBhojwani August 6, 2020 20:37
@DHowett DHowett changed the title pass mouse button state into HandleMouse Pass mouse button state into HandleMouse instead of asking win32 Aug 7, 2020
@DHowett DHowett merged commit 20a2880 into master Aug 7, 2020
DHowett pushed a commit that referenced this pull request Aug 11, 2020
@carlos-zamora @DHowett
Pass mouse button state into HandleMouse instead of asking win32 (#6765)
61234f6 
MouseInput was directly asking user32 about the state of the mouse buttons,
which was somewhat of a layering violation. This commit makes all callers
have to pass the mouse state in themselves.

Closes #4869

(cherry picked from commit 20a2880)
DHowett added a commit that referenced this pull request Aug 11, 2020
@DHowett
tools: add Get-OSSConhostLog
7f9f38e 
This script takes a range of commits and generates a commit log with the
git2git-excluded file changes filtered out.

It also replaces GitHub issue numbers with GH-XXX so as to not confuse
Git2Git or Azure DevOps.  Community contributions are tagged with CC- so
they can be detected later.

The output looks like this:

```
Carlos Zamora (2)
* Pass mouse button state into HandleMouse instead of asking win32 (GH-6765)

Dustin L. Howett (6)
* Disable MinimalCoreWin when OpenConsoleUniversalApp is false (GH-7203)

James Holderness (1)
* Add support for the "doubly underlined" graphic rendition attribute (CC-7223)
```
@DHowett DHowett mentioned this pull request Aug 11, 2020
Merged
ghost pushed a commit that referenced this pull request Aug 11, 2020
@DHowett
tools: add Get-OSSConhostLog (#7250)
c5d5500 
This script takes a range of commits and generates a commit log with the
git2git-excluded file changes filtered out.

It also replaces GitHub issue numbers with GH-XXX so as to not confuse
Git2Git or Azure DevOps.  Community contributions are tagged with CC- so
they can be detected later.

The output looks like this:

```
Carlos Zamora (2)
* Pass mouse button state into HandleMouse instead of asking win32 (GH-6765)

Dustin L. Howett (6)
* Disable MinimalCoreWin when OpenConsoleUniversalApp is false (GH-7203)

James Holderness (1)
* Add support for the "doubly underlined" graphic rendition attribute (CC-7223)
```

Yes, the numbers are wrong. No, it doesn't really matter.
DHowett added a commit that referenced this pull request Aug 15, 2020
@DHowett
Merged PR 5039910: Migrate OSS up to a2721c1
e1cdc27 
Carlos Zamora (1)
* Pass mouse button state into HandleMouse instead of asking win32 (GH-6765)

James Holderness (1)
* Add support for the "doubly underlined" graphic rendition attribute (CC-7223)

Moshe Schorr (1)
* Batch RTL runs to ensure proper draw order (CC-7190)

Related work items: MSFT-28385436
@DHowett DHowett deleted the dev/cazamor/vtmm/pass-button-state branch February 5, 2021 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@miniksa miniksa Awaiting requested review from miniksa

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees

@DHowett DHowett

Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Input Related to input processing (key presses, mouse, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Conpty For console issues specifically related to conpty
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pass Mouse Button State into HandleMouse
3 participants
@carlos-zamora @DHowett @zadjii-msft